Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore: deletes content temporary file on close. #924

Merged
merged 1 commit into from
May 9, 2024
Merged

chore: deletes content temporary file on close. #924

merged 1 commit into from
May 9, 2024

Conversation

jcchavezs
Copy link
Member

Fixes #922.

cc @UnveilTech

@jcchavezs jcchavezs requested a review from a team as a code owner November 21, 2023 05:54
@jptosso jptosso mentioned this pull request May 6, 2024
Closed
@victoredvardsson
Copy link

Bumping this one, would be nice if could be fixed. Right now i have to delete body* and crzmp* and body* manually in our env.

@jcchavezs
Copy link
Member Author

I will work on this tomorrow

@jcchavezs jcchavezs force-pushed the 922_deletes_tmp_content_on_close branch from d18d3a5 to d4de963 Compare May 9, 2024 07:07
@jcchavezs jcchavezs force-pushed the 922_deletes_tmp_content_on_close branch from d4de963 to 1455f1d Compare May 9, 2024 07:58
@jcchavezs
Copy link
Member Author

This is green. I will be merging it soon.

@jcchavezs jcchavezs added the v3.2 label May 9, 2024
M4tteoP
M4tteoP approved these changes May 9, 2024
View reviewed changes
Copy link
Member

@M4tteoP M4tteoP left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The initial Issue this PR meant to fix was about crzmp* files, generated by the multipart body processor. Just wanted to raise that we might have to double-check body* files:

coraza/internal/corazawaf/body_buffer.go

Line 78 in 5fd8d62

br.writer, err = os.CreateTemp(br.options.TmpPath, "body*")

I think that they are not added to filesTmpNames being really just a way to buffer the body on disk, and not files meant for @inspectFile. Therefore, we might still have to delete these files

That being said, looks good to me!

@jcchavezs
Copy link
Member Author

Just wanted to raise that we might have to double-check body* files

Good point, yeah we delete them already on reset (when transaction is being closed):

coraza/internal/corazawaf/body_buffer.go

Line 168 in 1455f1d

return os.Remove(w.Name())

@jcchavezs jcchavezs merged commit d0e219e into main May 9, 2024
7 checks passed
@jcchavezs jcchavezs deleted the 922_deletes_tmp_content_on_close branch May 9, 2024 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@M4tteoP M4tteoP M4tteoP approved these changes

Assignees
No one assigned
Labels
v3.2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

/tmp "crzmp*" never deleted after a POST
3 participants
@jcchavezs @victoredvardsson @M4tteoP